[TRTLLM-14806][feat] Prefer Python V2 transceiver for LlamaForCausalLM and Gemma3ForCausalLM - #16787
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughLlama and Gemma causal language model classes now declare Python as their preferred transceiver runtime. Tests cover automatic resolution, while disaggregated tests explicitly select UCX or the C++ serialization protocol. ChangesTransceiver runtime preferences
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/models/modeling_gemma3.py (1)
279-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument both resolver-facing runtime-preference hooks.
Add matching Google-style docstrings documenting
pretrained_configand the returned runtime sentinel:
tensorrt_llm/_torch/models/modeling_gemma3.py#L279-L284: documentGemma3ForCausalLM.get_preferred_transceiver_runtime.tensorrt_llm/_torch/models/modeling_llama.py#L1137-L1142: documentLlamaForCausalLM.get_preferred_transceiver_runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/models/modeling_gemma3.py` around lines 279 - 284, Add matching Google-style docstrings to Gemma3ForCausalLM.get_preferred_transceiver_runtime in tensorrt_llm/_torch/models/modeling_gemma3.py lines 279-284 and LlamaForCausalLM.get_preferred_transceiver_runtime in tensorrt_llm/_torch/models/modeling_llama.py lines 1137-1142, documenting the pretrained_config parameter and the returned "PYTHON" runtime sentinel while preserving the existing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_gemma3.py`:
- Around line 279-284: Add matching Google-style docstrings to
Gemma3ForCausalLM.get_preferred_transceiver_runtime in
tensorrt_llm/_torch/models/modeling_gemma3.py lines 279-284 and
LlamaForCausalLM.get_preferred_transceiver_runtime in
tensorrt_llm/_torch/models/modeling_llama.py lines 1137-1142, documenting the
pretrained_config parameter and the returned "PYTHON" runtime sentinel while
preserving the existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 71444888-d1ad-47d4-8c6b-dcf66b86a774
📒 Files selected for processing (3)
tensorrt_llm/_torch/models/modeling_gemma3.pytensorrt_llm/_torch/models/modeling_llama.pytests/unittest/llmapi/test_llm_args.py
|
Could you please check whether any existing tests need to be adjusted, similar to the changes made in PR 16482? |
I checked this, and the situation is slightly different from PR 16482. The existing Kimi test used DEFAULT, which caused the test harness to fall back to UCX/CPP, so it had to be explicitly changed to NIXL + PYTHON. Llama and Gemma already have corresponding NIXL + PYTHON disaggregated accuracy tests, so I don’t think similar test changes are needed here. |
97231af to
3b8aeaa
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #61293 [ run ] triggered by Bot. Commit: |
I only updated one test using |
|
/bot run --disable-fail-fast |
|
PR_Github #61343 [ run ] triggered by Bot. Commit: |
|
PR_Github #61293 [ run ] completed with state
|
|
PR_Github #61343 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61484 [ run ] triggered by Bot. Commit: |
|
PR_Github #61484 [ run ] completed with state
|
|
PR_Github #64724 [ run ] triggered by Bot. Commit: |
|
PR_Github #64724 [ run ] completed with state |
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
Signed-off-by: Michal Guzek <mguzek@nvidia.com>
3beb801 to
d2d260d
Compare
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #65137 [ run ] triggered by Bot. Commit: |
|
PR_Github #65137 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65214 [ run ] triggered by Bot. Commit: |
|
PR_Github #65214 [ run ] completed with state
|
|
/bot run |
|
PR_Github #65333 [ run ] triggered by Bot. Commit: |
|
PR_Github #65333 [ run ] completed with state |
…VIDIA#16787) Signed-off-by: Eric Tsai <ertsai@nvidia.com>
…VIDIA#16787) Signed-off-by: Eric Tsai <ertsai@nvidia.com>
…VIDIA#16787) Signed-off-by: Eric Tsai <ertsai@nvidia.com>
Dev Engineer Review
LlamaForCausalLMandGemma3ForCausalLMnow prefer the Python V2 transceiver whentransceiver_runtimeis left atautoand the effective backend is NIXL.Llama4ForConditionalGeneration,MistralForCausalLM, andGemma3ForConditionalGenerationare unchanged.DataTransceiverStateprotocol and differs from C++ in experimental in-flight cancellation and synchronous timing output; those C++-specific tests remain on UCX/CPP, while the UCX logprobs RCCA path also remains unchanged.QA Engineer Review
autoresolves toPYTHONfor both model classes without overriding explicit runtime choices or selecting Python for unsupported backends.Description
Prefer V2 transceiver backend for Gemma and Llama
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.